feat: handle parameterConsistency option in YAML extensions #624

Adam-Alani · 2025-11-27T09:50:16Z

This pull request introduces improved handling and validation of variadic parameter consistency for function signatures, particularly distinguishing between consistent and inconsistent parameter types in variadic arguments. The changes ensure that function argument matching logic respects the parameterConsistency setting and are thoroughly tested with new unit tests.

Enhancements to variadic parameter consistency:

Updated the function argument matching logic in FunctionConverter so that, when parameterConsistency is set to CONSISTENT, all variadic arguments must have the same type (ignoring nullability), and when set to INCONSISTENT, variadic arguments can differ in type. This logic is now explicitly enforced in the inputTypesMatchDefinedArguments method. [1] [2] [3]
Added a default value for parameterConsistency in the SimpleExtension.VariadicBehavior interface, defaulting to CONSISTENT if not specified.

FIxes: #622

vbarua

Left some comments. The big question I have is actually around what INCONSISTENT is supposed to mean. It's not super clear to me based on what's currently in the specification, and I'm not sure if the checks you've added in the FunctionConverter are correct. We can follow up with the upstream to clarify the intended behaviour for the Isthmus code.

The code you've added to the core to parse the parameterConsistency looks good to me, and stops substrait-java from blowing up if it encounters an extension with parameter consistency set as Ben pointed out in #622. If you make the core parsing changes their own PR, I'd be happy to merge those in.

isthmus/src/test/java/io/substrait/isthmus/expression/VariadicParameterConsistencyTest.java

core/src/test/java/io/substrait/extension/TypeExtensionTest.java

vbarua · 2025-11-27T17:22:03Z

isthmus/src/main/java/io/substrait/isthmus/expression/FunctionConverter.java

-      //   have the same type.
-      return wildcardToType.values().stream().allMatch(s -> s.size() == 1);
+      // When parameterConsistency is INCONSISTENT, wildcard types can differ.
+      // When parameterConsistency is CONSISTENT (or not variadic), wildcard types must be the same.


As I understand it, the parameterConsistency behaviour only applies to the variadic arguments. It's not related to how we typecheck the enumerated wildcard types (i.e any1, any2, etc). From the (limited) documentation on it:

When the last argument of a function is variadic and declares a type parameter e.g. fn(A, B, C...), the C parameter can be marked as either consistent or inconsistent. If marked as consistent, the function can only be bound to arguments where all the C types are the same concrete type. If marked as inconsistent, each unique C can be bound to a different type within the constraints of what T allows.

CONSISTENT means that the types of all the variadic arguments have to be the same. INCONSISTENT means... I'm not sure tbh because in

each unique C can be bound to a different type within the constraints of what T allows.

I don't know what T is. This is something I can follow up with the community about.

Ah I realize that this is attempting to handle the second part of the ticket I linked (#622).

However, I don't believe this is the right place to handle it. Considering that parameterConsistency has meaning regardless of calcite, those checks should really be inside of core/ IMO.

vbarua · 2025-11-27T17:23:46Z

core/src/main/java/io/substrait/extension/SimpleExtension.java


+    @Value.Default
    default ParameterConsistency parameterConsistency() {
      return ParameterConsistency.CONSISTENT;


I think this is a reasonble default, because I think it's what most people expect in practice and it's also the first value in the enumeration.

We can formalize this in the spec more concretely.

benbellick

Thanks for this. Can you make sure to link #622 in your PR description? Also, do you plan on implementing the second piece of that ticket? Which is validating that variadic rguments to functions are consistent with the parameterConsistency argument. Let me know if you need me to clarify anything.

benbellick · 2025-12-01T22:44:10Z

isthmus/src/main/java/io/substrait/isthmus/expression/FunctionConverter.java

-      //   have the same type.
-      return wildcardToType.values().stream().allMatch(s -> s.size() == 1);
+      // When parameterConsistency is INCONSISTENT, wildcard types can differ.
+      // When parameterConsistency is CONSISTENT (or not variadic), wildcard types must be the same.


Ah I realize that this is attempting to handle the second part of the ticket I linked (#622).

However, I don't believe this is the right place to handle it. Considering that parameterConsistency has meaning regardless of calcite, those checks should really be inside of core/ IMO.

benbellick

A lot of my comments are about relatively minor things. But the most important point is about the meaning of INCONSISTENT. I do think that the upstream could benefit from a bit more clarity there. Thanks for your work and let me know if anything I have said is unclear!

benbellick · 2025-12-04T16:37:31Z

core/src/main/java/io/substrait/expression/VariadicParameterConsistencyValidator.java

+    SimpleExtension.VariadicBehavior variadicBehavior = variadic.get();
+    if (variadicBehavior.parameterConsistency()
+        != SimpleExtension.VariadicBehavior.ParameterConsistency.CONSISTENT) {
+      // INCONSISTENT allows different types, so validation passes


So unfortunately, this is going to be a bit more complicated than it initially seemed. From the docs:

// Each argument can be any possible concrete type afforded by the bounds // of any parameter defined in the arguments specification.

Let me show you a (made-up) example:

urn: "urn:example:extension" scalar_functions: - name: "inconsistent_sum" impls: - args: - value: "decimal<P,S>" variadic: min: 1 parameterConsistency: INCONSISTENT return: "decimal<38,S>"

This means that the following are valid:

inconsistent_sum(decimal<10, 2>, decimal<10, 2>, decimal<10, 2>)

inconsistent_sum(decimal<10, 2>, decimal<10, 2>)

inconsistent_sum(decimal<15, 8>, decimal<11, 8>, decimal<119, 8>)

On the other hand, the following are all invalid:

inconsistent_sum(decimal<10, 2>, decimal<10, 3>, decimal<10, 4>)

inconsistent_sum(decimal<10, 2>, i32)

The above example is showing that there is the implicit constraint across the variadic parameters that the scale S must be the same as the output (and thus must all be the same across the variadic parameter). On the other hand, the precision P is free to be anything. But all of the parameters do have to be decimal.

Not sure if that was instructive or not 😅

For readability, it may make sense to have two private methods that implement each behavior, then have the public method just be a switch on the parameter consistency options. Just an idea!

I'll open a different issue for this and a follow up PR, and we can add a big TODO comment

You might as well put the ticket number in the comment as well.

core/src/main/java/io/substrait/expression/VariadicParameterConsistencyValidator.java

benbellick · 2025-12-04T18:59:58Z

core/src/test/java/io/substrait/expression/VariadicParameterConsistencyTest.java

+            .options(java.util.Collections.emptyMap())
+            .build();
+
+    return Expression.ScalarFunctionInvocation.builder()


nit: can we use ExpressionCreator.scalarFunction here instead?

Sorry unresolving this one, did you maybe miss it?

benbellick · 2025-12-04T19:03:21Z

core/src/test/java/io/substrait/expression/VariadicParameterConsistencyTest.java

+  }
+
+  @Test
+  void testConsistentVariadicWithSameTypes() {


Basically every place here where you are using the builders directly, I think it would be better to use the ExpressionCreator helpers. E.g.
Expression.I64Literal.builder().value(1).build() -> ExpressionCreator.i8(false, 1)

Also unresolving this one, did you maybe miss it?

benbellick · 2025-12-04T19:05:12Z

core/src/test/java/io/substrait/extension/VariadicBehaviorTest.java

+
+    assertEquals(
+        SimpleExtension.VariadicBehavior.ParameterConsistency.CONSISTENT,
+        collection.scalarFunctions().get(0).variadic().get().parameterConsistency());


Lets be super thorough and also add a test to make sure that parameterConsistency: INCONSISTENT is also passed through.

Also unresolving this one, think its also missed.

core/src/test/java/io/substrait/expression/VariadicParameterConsistencyTest.java

benbellick

Left a few more comments. Thanks!

benbellick · 2025-12-09T15:18:52Z

core/src/main/java/io/substrait/expression/VariadicParameterConsistencyValidator.java

+ * that when parameterConsistency is CONSISTENT, all variadic arguments have the same type (ignoring
+ * nullability).
+ */
+public class VariadicParameterConsistencyValidator {


Suggested change

public class VariadicParameterConsistencyValidator {

class VariadicParameterConsistencyValidator {

This way we keep it package private.

benbellick · 2025-12-09T15:19:13Z

core/src/main/java/io/substrait/expression/VariadicParameterConsistencyValidator.java

+    SimpleExtension.VariadicBehavior variadicBehavior = variadic.get();
+    if (variadicBehavior.parameterConsistency()
+        != SimpleExtension.VariadicBehavior.ParameterConsistency.CONSISTENT) {
+      // INCONSISTENT allows different types, so validation passes


You might as well put the ticket number in the comment as well.

benbellick · 2025-12-09T16:05:58Z

core/src/main/java/io/substrait/expression/AggregateFunctionInvocation.java

+  /**
+   * Validates that variadic arguments satisfy the parameter consistency requirement. When
+   * CONSISTENT, all variadic arguments must have the same type (ignoring nullability). When
+   * INCONSISTENT, arguments can have different types.


Lets clarify this to say something other than "have different types". Maybe, "have any type satisfying the type constraints" or something?

benbellick · 2025-12-09T16:13:19Z

core/src/main/java/io/substrait/expression/VariadicParameterConsistencyValidator.java

+                    return (Type) arg;
+                  }
+                })
+            .collect(Collectors.toList());


I wonder if all of this filtering logic could be simplified if we just introduced an EnumArg type?

I think its fine to leave this for now, but just making note of it.

benbellick · 2025-12-09T16:28:51Z

core/src/main/java/io/substrait/expression/VariadicParameterConsistencyValidator.java

+    // Count how many Expression/Type arguments are in the fixed arguments (before variadic)
+    // Note: func.args() includes all argument types (Expression, Type, EnumArg), but we only
+    // care about Expression/Type arguments for type consistency checking
+    int fixedTypeArgCount = 0;


I think a better name may be something like nonVariadicArgCount? It would be nice to if you had an example e.g. sum(i32...) has a nonVariadicArgCount of 0. Or is it 1?

The reason why I am suggested an example is just that it isn't entirely clear to me whether or not the first non-variadic example is included in this list or not without closely reading the code.

benbellick · 2025-12-09T16:35:26Z

core/src/main/java/io/substrait/expression/VariadicParameterConsistencyValidator.java

+    int fixedTypeArgCount = 0;
+    for (int i = 0; i < func.args().size() && i < arguments.size(); i++) {
+      FunctionArg arg = arguments.get(i);
+      if (arg instanceof Expression || arg instanceof Type) {
+        fixedTypeArgCount++;
+      }
+    }


Take it or leave it, but since you are already doing it functional style already above. But yeah truly feel free to ignore if these seems more confusing.

Suggested change

int fixedTypeArgCount = 0;

for (int i = 0; i < func.args().size() && i < arguments.size(); i++) {

FunctionArg arg = arguments.get(i);

if (arg instanceof Expression || arg instanceof Type) {

fixedTypeArgCount++;

}

}

int fixedTypeArgCount =

(int)

arguments.stream()

.limit(func.args().size())

.filter(arg -> arg instanceof Expression || arg instanceof Type)

.count();

benbellick · 2025-12-09T16:41:59Z

core/src/test/java/io/substrait/expression/VariadicParameterConsistencyTest.java

+            .options(java.util.Collections.emptyMap())
+            .build();
+
+    return Expression.ScalarFunctionInvocation.builder()


Sorry unresolving this one, did you maybe miss it?

benbellick · 2025-12-09T16:42:29Z

core/src/test/java/io/substrait/expression/VariadicParameterConsistencyTest.java

+  }
+
+  @Test
+  void testConsistentVariadicWithSameTypes() {


Also unresolving this one, did you maybe miss it?

benbellick · 2025-12-09T16:43:12Z

core/src/test/java/io/substrait/extension/VariadicBehaviorTest.java

+
+    assertEquals(
+        SimpleExtension.VariadicBehavior.ParameterConsistency.CONSISTENT,
+        collection.scalarFunctions().get(0).variadic().get().parameterConsistency());


Also unresolving this one, think its also missed.

Adam-Alani added 2 commits November 27, 2025 10:47

added parameterConsistency

e54b4f2

fmt

033f093

vbarua reviewed Nov 27, 2025

View reviewed changes

vbarua changed the title ~~Add parameterConsistency option to variadic behaviour~~ feat: handle parameterConsistency option in YAML extensions Nov 27, 2025

benbellick reviewed Dec 1, 2025

View reviewed changes

benbellick requested changes Dec 1, 2025

View reviewed changes

Adam-Alani added 2 commits December 4, 2025 10:52

applying comments

f8abdcd

simplify to class

718053a

Adam-Alani requested a review from benbellick December 4, 2025 12:38

benbellick requested changes Dec 4, 2025

View reviewed changes

Adam-Alani added 3 commits December 5, 2025 10:24

nit: comments

1eeed90

type comparison simplification

0d8c1b6

fix: lint and tests

e0ca93b

Adam-Alani requested a review from benbellick December 9, 2025 10:01

benbellick reviewed Dec 9, 2025

View reviewed changes

	public class VariadicParameterConsistencyValidator {
	class VariadicParameterConsistencyValidator {

feat: handle parameterConsistency option in YAML extensions #624

Are you sure you want to change the base?

feat: handle parameterConsistency option in YAML extensions #624

Uh oh!

Conversation

Adam-Alani commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vbarua left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benbellick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benbellick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

benbellick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Adam-Alani commented Nov 27, 2025 •

edited

Loading